-
-
Notifications
You must be signed in to change notification settings - Fork 70
Type annotation infrastructure and initial typing for qtbot.py #605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @herobank110!
Left some comments, please take a look!
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @herobank110, this looks great!
Thanks @herobank110! Leaving it open for a few days to give @The-Compiler a chance to review too. 👍 |
Keen to continue contributing with the typing and other stuff you may have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I'm pretty excited for this! ✨
I added a couple of comments for things I noticed, mostly they're minor things and shouldn't block anything though. A todo-list for follow-ups if we don't do them as part of this PR:
- Use Type Hinting Generics in Standard Collections (
list
,dict
,type
overtyping.List
etc.) - Use
from __future__ import annotations
- Use PEP 604: New Type Union Operator, i.e.
TheType | None
instead ofOptional[TheType]
- Use "bare" annotations for forward declarations instead of stringified ones
- Configure ruff to enforce
from __future__ import annotations
on every file for consistency
- Use PEP 604: New Type Union Operator, i.e.
- Remove redundant type annotations from Sphinx docstrings (and possibly configure Sphinx to pick up the annotations, not sure what's needed for that nowadays)
@@ -0,0 +1,9 @@ | |||
[mypy] | |||
exclude = ^docs/ | |||
no_implicit_optional = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already the default since mypy 0.980 (~September 2022).
CapturedException = Tuple[Type[BaseException], BaseException, TracebackType] | ||
CapturedExceptions = List[CapturedException] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CapturedException = Tuple[Type[BaseException], BaseException, TracebackType] | |
CapturedExceptions = List[CapturedException] | |
CapturedException = tuple[type[BaseException], BaseException, TracebackType] | |
CapturedExceptions = list[CapturedException] |
(possible since Python 3.9 which is now the earliest version we support, and avoids having to import all that stuff from typing
).
@@ -358,13 +403,13 @@ def waitSignal(self, signal, *, timeout=5000, raising=None, check_params_cb=None | |||
|
|||
def waitSignals( | |||
self, | |||
signals, | |||
signals: List[SignalInstance], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
signals: List[SignalInstance], | |
signals: list[SignalInstance], |
): | ||
timeout: int = 5000, | ||
raising: Optional[bool] = None, | ||
check_params_cbs: Optional[List[CheckParamsCb]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_params_cbs: Optional[List[CheckParamsCb]] = None, | |
check_params_cbs: Optional[list[CheckParamsCb]] = None, |
# provide easy access to exceptions to qtbot fixtures | ||
SignalEmittedError = SignalEmittedError | ||
TimeoutError = TimeoutError | ||
ScreenshotError = ScreenshotError | ||
CallbackCalledTwiceError = CallbackCalledTwiceError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those seem somewhat confusing - I wonder if we should from ... import SignalEmittedError as SignalEmittedError_
or something to make clear what happens here?
|
||
def _add_widget(item, widget, *, before_close_func=None): | ||
def _add_widget( | ||
item: Any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
item: Any, | |
item: pytest.Item, |
and add a type ignore about us monkeypatching stuff on it? Long-term, we should be using Stash instead anyways.
Same also for the other utility methods taking item
below.
@@ -769,7 +824,9 @@ class _WaitWidgetContextManager: | |||
Context manager implementation used by ``waitActive`` and ``waitExposed`` methods. | |||
""" | |||
|
|||
def __init__(self, method_name, adjective_name, widget, timeout): | |||
def __init__( | |||
self, method_name: str, adjective_name: str, widget: QWidget, timeout: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self, method_name: str, adjective_name: str, widget: QWidget, timeout: int | |
self, method_name: str, adjective_name: Literal["activated", "exposed"], widget: QWidget, timeout: int |
maybe, based on the docstring?
def __exit__(self, exc_type, exc_val, exc_tb): | ||
def __exit__( | ||
self, | ||
exc_type: Optional[Type[BaseException]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exc_type: Optional[Type[BaseException]], | |
exc_type: Optional[type[BaseException]], |
@@ -1,10 +1,12 @@ | |||
import functools | |||
import dataclasses | |||
from typing import Any | |||
from typing import Any, Callable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing.Callable
is deprecated, this should be using collections.abc.Callable
instead.
import weakref | ||
import warnings | ||
from typing import ( | ||
TYPE_CHECKING, | ||
Callable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing.Callable
is deprecated, this should be using collections.abc.Callable
instead.
Initial version of adding type annotations to the library.
infrastructure of mypy in precommit (ci and local type checking)
Typing focused around qtbot.py to start with
Note: qt objects are typed with aliases refering to Any - this may be upgraded in future to use correct types.
Fixes #417